-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate to dhall 1.26.1 and merge types and terms #54
Conversation
c8204ea
to
1a9a02a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t exports.dhall
be named package.dhall
to comply with the convention in Dhall prelude?
ah great point, I wasn't aware of this convention. I'll make the change |
(if there are other conventions I'm not following, please tell me) |
8f51cc1
to
0505d02
Compare
0505d02
to
2acb3d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like files without extensions (it can be confusing for new people coming on the dhall codebase) would you mind keeping all dhall files with the .dhall
extension?
« je vois que nous en sommes arrivés à la même conclusion, avec cependant un léger avantage pour moi : j'ai remis les .dhall » (extremely OSS117 voice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'll approve but please try to do an end-to-end test before merging :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a nice change!
Yeah, I'm quite happy with it :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will happily approve now that an end-to-end test was performed (and succeeded)
The latest dhall version allows to have types and terms in a given record. That allows to use records as proper modules without having separate files for types and terms.
Since all the files are small here, I merged related types and terms. I think it improves clarity (by improving locality)
Things to look at
I've loosely followed the conventions used in
Prelude
(https://github.com/dhall-lang/dhall-lang/tree/master/Prelude):package.dhall
file per dirType
What I did not follow:
Addon
andConfig
, the type is defined inline instead of in a separate file. The multiple files containing a single line made navigating ourcodeconfigbase tedious, so improving locality is a net improvement imo.dhall
extension (omitting it would break our tooling or would require brittle config rules)ToDo
I'd like to test an end-to-end